Add CustomEnityLookupSkill to 2020-06-30 and 2020-06-30-Preview #11113
Conversation
Remove unnecessary x-nullable
…0-Preview/searchservice.json Co-authored-by: Heath Stewart <heaths@outlook.com>
shuyangmsft
requested review from
AlexGhiondea,
arv100kri,
bleroy,
brjohnstmsft and
heaths
as code owners
Swagger Validation Report
|
|
Azure Pipelines successfully started running 1 pipeline(s). |
Azure CLI Extension Generation
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
azure-sdk-for-js
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
azure-sdk-for-java
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
azure-resource-manager-schemas
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
azure-sdk-for-python
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
Trenton Generation
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
azure-sdk-for-net
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
azure-sdk-for-go
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
| @@ -6943,6 +6943,92 @@ | |||
| }, | |||
| "description": "Base type for skills." | |||
| }, | |||
| "CustomEntity": { | |||
There was a problem hiding this comment.
2020-06-30 already GA'd. Unless this support already existed and just wasn't formalized in swagger, it should only be in 2020-06-30-Preview, which will change dates when closer to its GA.
There was a problem hiding this comment.
I actually have a question here given our case is a little bit involved.
The CustomEntityLookupSkill is publicly available. It's currently in our preview API and customers are using it right now.
The skill was added a while ago so it's not in the preview swagger.
Now our team wants to GA the feature so it should live in both the GA API and the preview API.
Given the situation we are at, should I remove or keep the 2020-06-30 change?
There was a problem hiding this comment.
hi team, can I get any update on this?
There was a problem hiding this comment.
FYI @bleroy since this is about Skillsets
There was a problem hiding this comment.
@heaths of course. We are still getting used to navigating this new world where the SDK team is so involved and all of the new breaking change requirements. We are definitely considering the SDK implications with the new skill versioning discussions. But since those discussions are ongoing and the work won't be done until Cobalt, I think taking a minor version bump for a change such as this one is acceptable for now.
There was a problem hiding this comment.
We're going to GA a minor version bump in a few weeks. @AlexGhiondea would adding a model require going through at least a single preview, or would this be minor enough (no pun intended) to include in our upcoming GA?
There was a problem hiding this comment.
@johanste how should this be handled going forward if new models are added to GAs? Isn't there roundtripping concerns here as well?
There was a problem hiding this comment.
Yes, there are some round-tripping concerns here. A client that knows nothing about a new skill type can easily end up stripping out properties it did not know about on update (assuming PUT does a replace rather than a merge-patch like update). I believe that I and @JeffreyRichter can be of assistance when it comes to determining (or at least reviewing) what a good versioning model for your service APIs should be.
Likewise, as mentioned above,
There was a problem hiding this comment.
- @jennifermarsman since I know she was talking about revisting this design in the coming weeks. We are hoping to finalize the design in Iron with the intent of implementing it in Cobalt, and we would definitely value any feedback from the SDK folks as we do so, so perhaps we should loop ya'll into our thread we have on the topic.
specification/search/data-plane/Azure.Search/preview/2020-06-30-Preview/searchservice.json
Show resolved
Hide resolved
specification/search/data-plane/Azure.Search/preview/2020-06-30-Preview/searchservice.json
Show resolved
Hide resolved
specification/search/data-plane/Azure.Search/preview/2020-06-30-Preview/searchservice.json
Show resolved
Hide resolved
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@shuyangmsft is this ready to merge? |
|
|
Azure Pipelines successfully started running 1 pipeline(s). |
Hi Travis, our backend change is in pipeline. I'll let you know when it's deployed to prod. Thanks! |
|
Any ETA? We have an SDK release coming up very soon and still need to regenerate source based on this change. |
Based on our pipeline status, we definitely can't make this change to prod this week. Next week should work. I'll keep you updated. |
heaths
added
the
DoNotMerge
|
Please hold off on merging this. We won't be able to generate and test this sufficiently for the upcoming release and have decided not to include it. It will have to be included in the next beta. This can be merged after we code freeze next week. |
|
Hi, @shuyangmsft. Your PR has no update for 14 days and it is marked as stale PR. If no further update for over 14 days, the bot will close the PR. If you want to refresh the PR, please remove |
ghost
added
the
heaths
removed
the
DoNotMerge
|
/azp run |
|
Swagger pipeline restarted successfully, please wait for status update in this comment. |
|
Azure Pipelines successfully started running 2 pipeline(s). |
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
Contribution checklist:
If any further question about AME onboarding or validation tools, please view the FAQ.
ARM API Review Checklist
Ensure to check this box if one of the following scenarios meet updates in the PR, so that label “WaitForARMFeedback” will be added automatically to involve ARM API Review. Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs, all “removals” and “adding a new property” no more require ARM API review.
If you are blocked on ARM review and want to get the PR merged with urgency, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
Breaking Change Review Checklist
If there are following updates in the PR, ensure to request an approval from API Review Board as defined in the Breaking Change Policy.
Please follow the link to find more details on PR review process.